feat: Add BeforeSendFeedback callback to inspect, modify, or drop user feedback before it's sent#5361
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5361 +/- ##
==========================================
+ Coverage 74.14% 74.19% +0.04%
==========================================
Files 509 509
Lines 18386 18409 +23
Branches 3600 3603 +3
==========================================
+ Hits 13633 13658 +25
+ Misses 3880 3879 -1
+ Partials 873 872 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…r feedback before it's sent Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…semantics Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1f51fe2 to
7bbbb1c
Compare
jamescrosswell
left a comment
jamescrosswell
left a comment
There was a problem hiding this comment.
Thanks for the PR @vladbrincoveanu ❤️
This looks all good to me. Nothing blocking from my side - I did have one question for you that might help guide the direction of these APIs in the future though.
| /// Return null or throw to drop the feedback. | ||
| /// </summary> | ||
| /// <param name="beforeSendFeedback">The callback</param> | ||
| public void SetBeforeSendFeedback(Func<SentryEvent, SentryEvent?> beforeSendFeedback) |
There was a problem hiding this comment.
More of a question than a comment, but I'm not sure if we need this overload. Originally we added an overload on the BeforeSend event when we wanted to introduce a variant that accepted a SentryHint parameter without that being a breaking change... we never made a conscious decision that we wanted two overloads of the callback though.
If we only had a single version of the callback that accepted both a SentryEvent and a SentryHint argument, users could easily ignore the second argument.
On the other hand, if we do that now for this callback, SetBeforeSendFeedback is then the odd one out (inconsistent)... so probably not something we want to do in this PR but something we could consider for all of the BeforeSend* callbacks as a breaking change in the next major:
- Do we want to simplify the BeforeSend* callbacks so that these are not overloaded?
@Flash0ver @vladbrincoveanu what are your thoughts?
There was a problem hiding this comment.
These overloads are just adapters. They just use the SentryHint version and their docs hide half the feature.
I agree, we can batch mark them as [Obsolete] for a next minor and remove them in the future major, so users gets some warnings first. This could be another good starter ticket, for cleanups.
3 concerns:
- the overload without SentryHint is the one used in docs, we need to updated the entries too.
- by marking them as obsolete first in a minor vers, the consumers either get a warning or their build can actually break if they have TreatWarningsAsErrors set up. Is this acceptable?
- SetBeforeSendLog and SetBeforeSendMetric do not have 2 overloads and we need to decide if it makes sense to add a SentryHint for logging and metrics.
1 suggestion:
- another possible doc update: new data for Before callbacks will go into SentryHint, not into a new delegate parameter, so we enforce it. Ofc, if we ever need to add a cancellationtoken or async, we will have a new overload.
There was a problem hiding this comment.
we can batch mark them as [Obsolete] for a next minor and remove them in the future major
Probably more effort that it's worth... The overloads don't add too much noise.
SetBeforeSendLog and SetBeforeSendMetric do not have 2 overloads and we need to decide if it makes sense to add a SentryHint for logging and metrics.
The main reason for the Hint parameter was to make it possible to pass in attachments... which I don't think make sense for metrics/logs (neither support attachments).
| } | ||
|
|
||
| [Fact] | ||
| public void CaptureFeedback_EnqueueFails_ReturnsUnknownError() |
There was a problem hiding this comment.
Seems unrelated to this PR but harmless...
"Refers" is not a verb that links a PR to an issue. GitHub links PRs to an issue by using any of these keywords: |
3 participants
Refers to #4092
Summary
Adds a
BeforeSendFeedbackcallback toSentryOptions, mirroring the existingBeforeSendpattern but scoped to user feedback events. It lets applications inspect, modify, or drop user feedback before it is sent to Sentry.Changes
SetBeforeSendFeedbackoverloads onSentryOptions(with and withoutSentryHint), consistent withSetBeforeSend/SetBeforeSendTransaction.SentryEventHelper.DoBeforeSendFeedback, invoked inSentryClient.CaptureFeedbackafter event processors run.CaptureFeedbackResult.DroppedByBeforeSendFeedbackresult valueClientReportRecorderunderDiscardReason.BeforeSend/DataCategory.Feedback. Originally, was specified the use of DataCategory USER_REPORT_V2.[SDK docs] add new UF data model and implementation details under telemetry dev docs sentry#88232 (comment)
Behavior notes for reviewers
Assumption: Feedback is a common place to scrub PII, so a callback that cannot complete should not leak partially-scrubbed or unintended feedback.
Implementation: Both the null-return and throw paths record a client-report discard. This is documented on the public API and covered by tests.
Tests
SentryClientTestscovers: callback invocation + mutation passthrough, hint received from caller, null and throw drops feedback.Unrelated changes
Added a small unit test for CaptureFeedbackResult.UnknownError.